feat(fast-usdc): detect transfer completion in cli #10717
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
2828444
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3c30b8b6.agoric-sdk.pages.dev |
| Branch Preview URL: | https://srs-fu-transfer-cli.agoric-sdk.pages.dev |
samsiegart
force-pushed
the
srs-fu-transfer-cli
branch
from
936020f to
3d341a8
Compare
packages/fast-usdc/src/util/bank.js
Outdated
| address, | ||
| api, | ||
| denom, | ||
| fetch = globalThis.fetch, |
There was a problem hiding this comment.
this is provided in every instance. please require it
| @@ -0,0 +1,14 @@ | |||
| /* global globalThis */ | |||
|
|
|||
| export const queryUSDCBalance = async ( | |||
There was a problem hiding this comment.
I'm getting used to this style vs @param. More like .ts and will make automatic conversions to that syntax easier.
packages/fast-usdc/src/util/bank.js
Outdated
| @@ -0,0 +1,14 @@ | |||
| /* global globalThis */ | |||
There was a problem hiding this comment.
in general prefer /* eslint-env node */ to indicate the environment a module expects.
see #8702
There was a problem hiding this comment.
Are they mutually exclusive? I want to use globalThis to namespace the global fetch, and have the arg default to that. I guess I'd have to rename it to fetchArg or something. But either way, I removed the default arg as you said, so it's moot.
There was a problem hiding this comment.
Are they mutually exclusive?
You mean is it okay to have both these lines?
/* global globalThis */
/* eslint-env node */
It doesn't cause harm but the first is accomplished by the second so it gives the reader pause as to why it's included.
Just updated the description. The x/forwarding stuff was implemented and tested in a previous PR (details there). This PR is just for detecting the transfer completion and reporting the time, which was the last part of the acceptance criteria for #10339. |
| @@ -0,0 +1,14 @@ | |||
| /* global globalThis */ | |||
|
|
|||
| export const queryUSDCBalance = async ( | |||
There was a problem hiding this comment.
I'm getting used to this style vs @param. More like .ts and will make automatic conversions to that syntax easier.
samsiegart
force-pushed
the
srs-fu-transfer-cli
branch
from
e54c0b4 to
2828444
Compare
fixes #10339
Description
This just simply polls the USDC balance on the destination account repeatedly to see when it changes
Security Considerations
There are ways to exploit this detection logic. Namely, if the user or someone else sends USDC to the EUD before the transfer finishes, the CLI would prematurely report that the transfer completed. We could refine this logic if needed, but it's just a UX issue and doesn't affect whether or not they get their funds.
Scaling Considerations
Polls the API url every 1.2 seconds temporarily, shouldn't be too much of an impact.
Documentation Considerations
Updated the demo config file with an example for osmosis chain.
Testing Considerations
We can't test this fully e2e until we have a real testnet setup with the FU contract and IBC to noble testnet and another cosmos chain. However, added unit tests to verify the new transfer detection functionality in this PR. The previously existing functionality of the transfer flow was partially tested in testnets when it was added in #10437 (see "Testing Considerations").
Upgrade Considerations
This CLI code does not run on-chain.